Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

support adding a CA cert to http collector #1624

Merged
merged 21 commits into from
Oct 23, 2024
Merged

support adding a CA cert to http collector #1624

merged 21 commits into from
Oct 23, 2024

Conversation

adamancini
Copy link
Member

@adamancini adamancini commented Sep 25, 2024 •
edited
Loading

Description, Motivation and Context

http collector now supports a cacert field and a proxy field, in order to support testing for man-in-the-middle proxies. This will be most useful in a HostPreflight scenario, checking whether the proxy CA cert provided by embedded-cluster install --private-ca can be used to successfully traverse the forward proxy. The expectation is that the proxy address and CA cert will be templated into a preflight spec at runtime.

These params are new and optional:

cacert: a path to a file, directory, or a literal string containing the CA certificate in PEM format

proxy: a URL to a proxy, including port and scheme: https://10.128.0.4:3130

E.g.:

- http:
        collectorName: https-replicated-proxy-cert-dir
        get:
          url: https://replicated.app
        tls:
          cacert: /usr/local/share/ca-certificates/replicated.app
        proxy: https://10.128.0.4:3130
    - http:
        collectorName: https-replicated-proxy-file
        get:
          url: https://replicated.app
        tls:
          cacert: /usr/local/share/ca-certificates/replicated.app/replicated.crt
        proxy: https://10.128.0.4:3130
    - http:
        collectorName: https-replicated-proxy-literal
        get:
          url: https://replicated.app
        tls:
          cacert: |-
            -----BEGIN CERTIFICATE-----
            MIIDajCCAlKgAwIBAgIUZJdSAOMxiY3cKU34KCtpK2ztkhUwDQYJKoZIhvcNAQEL
            BQAwTjELMAkGA1UEBhMCVVMxDjAMBgNVBAgMBVN0YXRlMQ0wCwYDVQQHDARDaXR5
            MRMwEQYDVQQKDApSZXBsaWNhdGVkMQswCQYDVQQLDAJJVDAeFw0yNDA5MjUxNDI0
            NTVaFw0yNDEwMDIxNDI0NTVaME4xCzAJBgNVBAYTAlVTMQ4wDAYDVQQIDAVTdGF0
            ZTENMAsGA1UEBwwEQ2l0eTETMBEGA1UECgwKUmVwbGljYXRlZDELMAkGA1UECwwC
            SVQwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDFaoJSv18TVLGFq2WC
            q6KOmlwfZpBXdgU1CYtg1HDwKUZ4/+WIcoHo+7yp+tt5TRA55r5d9YTlHWvQ+efh
            pRbIxmi7aTI9JyV9/jw3lDdVjShiNmdlNEGW7PYAihQkWLErMVcLspiZqmWB2Ar5
            bvkRGoLV8Mf7QdAIEPZzK3RxL9oQSk0F5T+n/TxEOlN3XCJxWvgZUFNslnTNkx9d
            ABeMBfoB22gJtcRqJ2Rsp3HrkNzOu4i5xHQtj3f4UdEkxBAUZwME1SK3qFVOfI4i
            Gon4jFW0y0NYRuzYTypBuLbGoQ3RcOmpLfOSzhZL/aEE95QXHNH3L6QzVF4kZUin
            7sIRAgMBAAGjQDA+MA8GA1UdEQQIMAaHBAqAAAQwDAYDVR0TBAUwAwEB/zAdBgNV
            HQ4EFgQUqJTkHiOkT65YvZelWCn/JziGN5AwDQYJKoZIhvcNAQELBQADggEBADPt
            pdL++z00Ix75MVmjxU7QeLorpSeiU1j4z26VgomkuiC6iJrMKrYOGBsn7uMmALO8
            lVHDUCfAHs0Jv1vijkczT6I+fJiRYz0gnXYfMlPTH3Z082wVB5i0/ZZzv2hbO/cH
            Z8nmQvX3S+vOH5t9s/af1lxYzRno4Bo5CRzV5yoWHJT0dkAL8Xq16PWl+KUp6hel
            pVoo6uhBGcaJK5HY0WczyUY6ecMJ2BEwc0jNyYBKdkLAB3110axgFvtdBweHsEQv
            ISeaclwXuzYuVFr0BawyUSjenl0t8ybbKFfNrLyv6FHTNiqRup6SxORr3eLSv4dZ
            6Qg1Og0ne2dGJrebKy0=
            -----END CERTIFICATE-----
        proxy: https://10.128.0.4:3130

Relates to #1302

Depends on replicatedhq/embedded-cluster#1228

Checklist

  • New and existing tests pass locally with introduced changes.
  • Tests for the changes have been added (for bug fixes / features)
  • The commit message(s) are informative and highlight any breaking changes
  • Any documentation required has been added/updated. For changes to https://troubleshoot.sh/ create a PR here
  • PR embedded-cluster to add a templated spec

Does this PR introduce a breaking change?

  • Yes
  • No

@adamancini adamancini added the type::feature New feature or request label Sep 25, 2024
banjoh
banjoh reviewed Sep 27, 2024
View reviewed changes
pkg/collect/host_http.go Outdated Show resolved Hide resolved
@adamancini adamancini mentioned this pull request Sep 27, 2024
Merged
@adamancini adamancini marked this pull request as ready for review September 27, 2024 19:02
@adamancini adamancini requested a review from a team as a code owner September 27, 2024 19:02
@banjoh
Copy link
Member

banjoh commented Oct 1, 2024

@adamancini make schemas is failing

@banjoh
Copy link
Member

banjoh commented Oct 1, 2024

Is proxy: still needed in the new implementation? I do not see the field defined anywhere in the code.

@adamancini
Copy link
Member Author

Is proxy: still needed in the new implementation? I do not see the field defined anywhere in the code.

yeah we use it here https://github.com/replicatedhq/troubleshoot/pull/1624/files#diff-23221d86b2950ee7b3125067dcade43eaecff05f4ea2654ff278d97bafdad2a7R138

@adamancini adamancini added the echoes/effort: M Medium effort changes by the team's standards (default) label Oct 10, 2024
@adamancini
Copy link
Member Author

added PR to embedded-cluster to add a spec to checklist - but this should be ready for review again @banjoh

@adamancini
Copy link
Member Author

@banjoh I'll follow up with another PR to clean up the code at https://github.com/replicatedhq/troubleshoot/pull/1624/files#diff-23221d86b2950ee7b3125067dcade43eaecff05f4ea2654ff278d97bafdad2a7R113

diamonwiggins
diamonwiggins reviewed Oct 23, 2024
View reviewed changes
.golangci.yaml
Comment on lines +14 to +15
disable:
- errcheck
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We intending to disable this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it intentionally because tests are failing on "you didn't check for a return error value" in the tests themselves

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/replicatedhq/troubleshoot/actions/runs/11486710569/job/31969600112?pr=1624#step:5:403

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like because of this most recent change 402d111

diamonwiggins
diamonwiggins approved these changes Oct 23, 2024
View reviewed changes
Copy link
Member

@diamonwiggins diamonwiggins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@diamonwiggins diamonwiggins merged commit eacff71 into main Oct 23, 2024
27 checks passed
@diamonwiggins diamonwiggins deleted the mitm-proxy branch October 23, 2024 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@diamonwiggins diamonwiggins diamonwiggins approved these changes

@banjoh banjoh Awaiting requested review from banjoh

Assignees
No one assigned
Labels
echoes/effort: M Medium effort changes by the team's standards (default) type::feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@adamancini @banjoh @diamonwiggins